- 
                Notifications
    
You must be signed in to change notification settings  - Fork 71
 
Combine x and ... in min, max, range #1946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
323a57d    to
    f8e15cc      
    Compare
  
    | 
           Sorry for the rewrite, fixed some commit metadata.  | 
    
| 
           Some notes: 
 Small-vctr benchmarks:# main
x <- new_vctr(c(1,2,4)); bench::mark(min(x), min_iterations = 1e5, max_iterations = 1e5)
#> # A tibble: 1 × 13
#>   expression      min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result    
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>    
#> 1 min(x)       50.6µs   55.9µs    17597.        NA     8.98 99949    51      5.68s <vctrs_vc>
#> # ℹ 3 more variables: memory <list>, time <list>, gc <list>
# this PR
x <- new_vctr(c(1,2,4)); bench::mark(min(x), min_iterations = 1e5, max_iterations = 1e5)
#> # A tibble: 1 × 13
#>   expression      min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result    
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>    
#> 1 min(x)       53.3µs   55.9µs    17608.        NA     8.98 99949    51      5.68s <vctrs_vc>
#> # ℹ 3 more variables: memory <list>, time <list>, gc <list>
# Without if-empty-dots optimization:
x <- new_vctr(c(1,2,4)); bench::mark(min(x), min_iterations = 1e5, max_iterations = 1e5)
#> # A tibble: 1 × 13
#>   expression      min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result    
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>    
#> 1 min(x)        187µs    207µs     4737.        NA     10.4 99780   220      21.1s <vctrs_vc>
#> # ℹ 3 more variables: memory <list>, time <list>, gc <list>
# Testing with short integer vectors (ALTREP seqs or not) also yielded comparable performance. | 
    
          
 I've gone ahead and implemented these checks for   | 
    
Only call `vec_c` if dots are nonempty to stay fast in the empty case.
The `range` generic doesn't separate out `x` and `...`, but splitting them out lets us easily check whether we can skip a potentially-slow `vec_c` operation to combine them.
d0c06d2    to
    8e0f62e      
    Compare
  
    | 
           Rebased onto current main to see if it will help with what seemed like unrelated failing checks. Coverage check still aborts immediately with 
 and I'm not sure how to resolve this. [(CHECK issues seem unrelated, so think this might still be in a mergeable state 
 )]  | 
    
Resolves #1372.
Only call
vec_c(x, ...)if dots are nonempty, in order to stay fast in the empty case.